Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Add processors to logging (placeholders) #46344

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

alanpoulain
Copy link
Contributor

Maybe this PR should target 11.x.

This PR adds the possibility to use Monolog processors and adds a default PsrLogMessageProcessor to all drivers.
Following @Crell's post, the logger should be able to use placeholders by default:

Log::info('Showing the user profile for user: {id}', ['id' => $id]);

It's not the case today so I added it in this PR.
It can be disabled with with_placeholders:

// logging.php

'channels' => [
    'single' => [
        'driver' => 'single',
        'path' => storage_path('logs/laravel.log'),
        'level' => env('LOG_LEVEL', 'debug'),
        'with_placeholders' => false,
    ],
],

This PR also adds the possibility to configure processors for the monolog driver:

// logging.php

'channels' => [
    'memory' => [
        'driver' => 'monolog',
        'handler' => Monolog\Handler\StreamHandler::class,
        'level' => 'notice',
        'with' => [
            'stream' => 'php://stderr',
        ],
        'processors' => [
            // simple syntax
            Monolog\Processor\MemoryUsageProcessor::class,
            // complex syntax
            ['processor' => Monolog\Processor\PsrLogMessageProcessor::class, 'with' => ['removeUsedContextFields' => true]],
        ],
    ],
],

@ankurk91
Copy link
Contributor

ankurk91 commented Mar 3, 2023

To preserve previous behaviour with_placeholders must be treated as false by framework, and let user opt in to this feature

@alanpoulain
Copy link
Contributor Author

alanpoulain commented Mar 3, 2023

Sure but if it's false by default, it contradicts what the post is saying, particularly in term of security:

Passing potentially user-provided data into a log message is a security risk, and the escaping mechanism needed for them is not known at call time.

I'm not sure it's really a breaking change, since the PsrLogMessageProcessor only replaces the placeholder if the value exists in the context.

@Crell
Copy link

Crell commented Mar 3, 2023

The behavior change would be that, currently, you get the following written to the log:

Showing the user profile for user: {id} ['id' => 5]

whereas with the placeholders interpolated, you'd get this:

Showing the user profile for user: 5 ['id' => 5]

The degree to which that's a breaking change is not my call to make. I will note, though, that whether you want the placeholders interpolated is going to vary by the handler. For writing to syslog, probably yes. For writing to a DB, almost certainly no. For writing to a remote logging service, maybe, depending on if the service supports placeholders.

So the default should probably vary by the handler used.

@dennisprudlo
Copy link
Contributor

The default can be false for 10.x and true for 11.x and above.

Then apps using 10.x can opt in, and apps that don't want to use this can opt out in 11.x

@alanpoulain
Copy link
Contributor Author

It could, sure (and I can do it), but it's worth the (very small) risk to do it now in my opinion.
But I will comply to what Laravel mergers say.

@taylorotwell
Copy link
Member

It's less breaking to default to false here in laravel/framework and update the skeleton to include replace_placeholders as true. That way it's only the default for new applications.

@kayw-geek
Copy link
Contributor

Maybe this PR should target 11.x.

This PR adds the possibility to use Monolog processors and adds a default PsrLogMessageProcessor to all drivers. Following @Crell's post, the logger should be able to use placeholders by default:

Log::info('Showing the user profile for user: {id}', ['id' => $id]);

It's not the case today so I added it in this PR. It can be disabled with with_placeholders:

// logging.php

'channels' => [
    'single' => [
        'driver' => 'single',
        'path' => storage_path('logs/laravel.log'),
        'level' => env('LOG_LEVEL', 'debug'),
        'with_placeholders' => false,
    ],
],

This PR also adds the possibility to configure processors for the monolog driver:

// logging.php

'channels' => [
    'memory' => [
        'driver' => 'monolog',
        'handler' => Monolog\Handler\StreamHandler::class,
        'level' => 'notice',
        'with' => [
            'stream' => 'php://stderr',
        ],
        'processors' => [
            // simple syntax
            Monolog\Processor\MemoryUsageProcessor::class,
            // complex syntax
            ['processor' => Monolog\Processor\PsrLogMessageProcessor::class, 'with' => ['removeUsedContextFields' => true]],
        ],
    ],
],

The with_placeholders is wrong, we should replace it using the replace_placeholders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants